Improve image moving error handling:

* Do image moving before page moving. That will allow to avoid situations when image page is moved, while image itself isn't.
* Add safeguard code that checks all files before moving them
* More debug logging

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 02a0fe1..f8a9bea 100644 (file)
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -308,6 +308,7 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN
 * (bug 18190) Proper parsing in MediaWiki:Sharedupload message
 * (bug 17617) HTML cleanup for ImagePage
 * (bug 17964) namespaceDupes.php no longer fails on an empty interwiki table
+* Improved error handling for image moving
 
 == API changes in 1.15 ==
 * (bug 16858) Revamped list=deletedrevs to make listing deleted contributions

diff --git a/includes/Status.php b/includes/Status.php
index 185ea6e..1eb2b66 100644 (file)
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -175,7 +175,10 @@ class Status {
                $result = array();
                foreach ( $this->errors as $error ) {
                        if ( $error['type'] == 'error' )
-                               $result[] = $error['message'];
+                               if( $error['params'] )
+                                       $result[] = array_merge( array( $error['message'] ), $error['params'] );
+                               else
+                                       $result[] = $error['message'];
                }
                return $result;
        }

diff --git a/includes/Title.php b/includes/Title.php
index 3ce99d8..782169c 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -2664,6 +2664,18 @@ class Title {
                        return $err;
                }
 
+               // If it is a file, more it first. It is done before all other moving stuff is done because it's hard to revert
+               $dbw = wfGetDB( DB_MASTER );
+               if( $this->getNamespace() == NS_FILE ) {
+                       $file = wfLocalFile( $this );
+                       if( $file->exists() ) {
+                               $status = $file->move( $nt );
+                               if( !$status->isOk() ) {
+                                       return $status->getErrorsArray();
+                               }
+                       }
+               }
+
                $pageid = $this->getArticleID();
                $protected = $this->isProtected();
                if( $nt->exists() ) {
@@ -2690,7 +2702,6 @@ class Title {
                // we can't actually distinguish it from a default here, and it'll
                // be set to the new title even though it really shouldn't.
                // It'll get corrected on the next edit, but resetting cl_timestamp.
-               $dbw = wfGetDB( DB_MASTER );
                $dbw->update( 'categorylinks',
                        array(
                                'cl_sortkey' => $nt->getPrefixedText(),
@@ -2873,18 +2884,6 @@ class Title {
                        $redirectSuppressed = true;
                }
 
-               # Move an image if this is a file
-               if( $this->getNamespace() == NS_FILE ) {
-                       $file = wfLocalFile( $this );
-                       if( $file->exists() ) {
-                               $status = $file->move( $nt );
-                               if( !$status->isOk() ) {
-                                       $dbw->rollback();
-                                       return $status->getErrorsArray();
-                               }
-                       }
-               }
-
                # Log the move
                $log = new LogPage( 'move' );
                $log->addEntry( 'move_redir', $this, $reason, array( 1 => $nt->getPrefixedText(), 2 => $redirectSuppressed ) );
@@ -2970,18 +2969,6 @@ class Title {
                        $redirectSuppressed = true;
                }
 
-               # Move an image if this is a file
-               if( $this->getNamespace() == NS_FILE ) {
-                       $file = wfLocalFile( $this );
-                       if( $file->exists() ) {
-                               $status = $file->move( $nt );
-                               if( !$status->isOk() ) {
-                                       $dbw->rollback();
-                                       return $status->getErrorsArray();
-                               }
-                       }
-               }
-
                # Log the move
                $log = new LogPage( 'move' );
                $log->addEntry( 'move', $this, $reason, array( 1 => $nt->getPrefixedText(), 2 => $redirectSuppressed ) );

diff --git a/includes/filerepo/FSRepo.php b/includes/filerepo/FSRepo.php
index d561e61..5a868e2 100644 (file)
--- a/includes/filerepo/FSRepo.php
+++ b/includes/filerepo/FSRepo.php
@@ -212,6 +212,33 @@ class FSRepo extends FileRepo {
                return $status;
        }
 
+       /**
+        * Checks existance of specified array of files.
+        *
+        * @param array $files URLs of files to check
+        * @param integer $flags Bitwise combination of the following flags:
+        *     self::FILES_ONLY     Mark file as existing only if it is a file (not directory)
+        * @return Either array of files and existance flags, or false
+        */
+       function fileExistsBatch( $files, $flags = 0 ) {
+               if ( !file_exists( $this->directory ) || !is_readable( $this->directory ) ) {
+                       return false;
+               }
+               $result = array();
+               foreach ( $files as $key => $file ) {
+                       if ( self::isVirtualUrl( $file ) ) {
+                               $file = $this->resolveVirtualUrl( $file );
+                       }
+                       if( $flags & self::FILES_ONLY ) {
+                               $result[$key] = is_file( $file );
+                       } else {
+                               $result[$key] = file_exists( $file );
+                       }
+               }
+
+               return $result;
+       }
+
        /**
         * Take all available measures to prevent web accessibility of new deleted
         * directories, in case the user has not configured offline storage

diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index face161..22e41d1 100644 (file)
--- a/includes/filerepo/FileRepo.php
+++ b/includes/filerepo/FileRepo.php
@@ -6,6 +6,7 @@
  * @ingroup FileRepo
  */
 abstract class FileRepo {
+       const FILES_ONLY = 1;
        const DELETE_SOURCE = 1;
        const FIND_PRIVATE = 1;
        const FIND_IGNORE_REDIRECT = 2;

diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php
index b997d75..710a605 100644 (file)
--- a/includes/filerepo/LocalFile.php
+++ b/includes/filerepo/LocalFile.php
@@ -1789,6 +1789,11 @@ class LocalFileMoveBatch {
                $status = $repo->newGood();
                $triplets = $this->getMoveTriplets();
 
+               $statusPreCheck = $this->checkFileExistance( 0 );
+               if( !$statusPreCheck->isOk() ) {
+                       wfDebugLog( 'imagemove', "Move of {$this->file->name} aborted due to pre-move file existance check failure" );
+                       return $statusPreCheck;
+               }
                $statusDb = $this->doDBUpdates();
                wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
                $statusMove = $repo->storeBatch( $triplets, FSRepo::DELETE_SOURCE );
@@ -1796,7 +1801,14 @@ class LocalFileMoveBatch {
                if( !$statusMove->isOk() ) {
                        wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
                        $this->db->rollback();
+               } else {
+                       $statusPostCheck = $this->checkFileExistance( 1 );
+                       if( !$statusPostCheck->isOk() ) {
+                               // This clearly mustn't have happend. FSRepo::storeBatch should have given out an error in that case.
+                               wfDebugLog( 'imagemove', "ATTENTION! Move of {$this->file->name} has some files missing, while storeBatch() reported success" );
+                       }
                }
+
                $status->merge( $statusDb );
                $status->merge( $statusMove );
                return $status;
@@ -1856,4 +1868,23 @@ class LocalFileMoveBatch {
                }
                return $triplets;
        }
+
+       /*
+        * Checks file existance.
+        * Set $key = 0 for source files check
+        * and $key = 1 for destination files check.
+        */ 
+       function checkFileExistance( $key = 0 ) {
+               $files = array();
+               foreach( array_merge( array( $this->cur ), $this->olds ) as $file )
+                       $files[$file[$key]] = $this->file->repo->getVirtualUrl() . '/public/' . rawurlencode( $file[$key] );
+               $result = $this->file->repo->fileExistsBatch( $files, FSRepo::FILES_ONLY );
+               $status = $this->file->repo->newGood();
+               foreach( $result as $filename => $exists )
+                       if( !$exists ) {
+                               wfDebugLog( 'imagemove', "File {$filename} does not exist" );
+                               $status->fatal( 'filenotfound', $filename );
+                       }
+               return $status;
+       }
 }